Skip to content

[IR Refactor] Fusion Base Type #5902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
mdavis36 wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

[IR Refactor] Fusion Base Type #5902

mdavis36 wants to merge 7 commits into from
+295 −417

Conversation

@mdavis36
Copy link
Collaborator

@mdavis36 mdavis36 commented Jan 30, 2026

No description provided.

@mdavis36
Copy link
Collaborator Author

mdavis36 commented Jan 30, 2026

!test

@github-actions
Copy link

github-actions bot commented Jan 30, 2026 •
edited
Loading

Review updated until commit f11f807

Description

  • Refactor Fusion to use composition over inheritance with IrContainer

  • Rename IrStorage class to IrContainer and move all functionality

  • Update all method signatures from IrContainer* to Fusion* pointers

  • Remove forwarding interface layer and directly access container methods

  • Clean up inheritance hierarchy and simplify class relationships

Changes walkthrough
Relevant files
Enhancement
15 files
fusion.cpp
Update Fusion methods to use composition pattern                 
+45/-10 
cloner.cpp
Change IrCloner to use Fusion* instead of IrContainer*     
+1/-1     
container.cpp
Remove entire file - forwarding interface eliminated         
+0/-57   
storage.cpp
Rename IrStorage to IrContainer and move all methods         
+27/-27 
utils.cpp
Update method signatures to use Fusion* instead of IrContainer*
 +1/-1     
mutator.cpp
Update OptOutMutator to use Fusion* instead of IrContainer*
 +1/-1     
dispatch.h
Update method signatures in dispatch interface                     
+1/-1     
fusion.h
Major refactor - Fusion contains IrContainer instead of inheriting
 +176/-16
base_nodes.h
Update Statement and related classes to use Fusion* instead of
IrContainer*
 +6/-12   
builder.h
Update template method signatures for new composition pattern
 +4/-9     
builder_passkey.h
Update passkey to use Fusion* instead of IrContainer*       
+4/-4     
cloner.h
Update IrCloner class to use Fusion* instead of IrContainer*
 +6/-32   
container.h
Remove forwarding interface - file now essentially empty 
+1/-226 
storage.h
Rename IrStorage to IrContainer and consolidate functionality
 +21/-16 
kernel.h
Remove inheritance-related using declarations                       
+0/-3     
Configuration changes
1 files
CMakeLists.txt
Comment out container.cpp from compilation                             
+1/-1     

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 PR contains tests
⚡ Recommended focus areas for review
Memory Management Safety

The Fusion class now owns an IrContainer via unique_ptr (line 669). This is a significant architectural change from inheritance to composition. Need to verify proper copy/move semantics are implemented correctly, especially for the ir_container_ member during Fusion copy/move operations. The current implementation in fusion.cpp shows copy constructor calls Fusion() then copy(), and move constructor calls Fusion() then swap(). Ensure these patterns properly handle the unique_ptr lifecycle and no memory leaks or double-frees occur.

std::unique_ptr<IrContainer> ir_container_;
Parent Pointer Type Mismatch

The parent_ member in IrContainer changed from IrContainer* to Fusion* (line 200), but Statement::ir_container_ member is still Fusion* (line 184 in base_nodes.h). This creates a potential type safety issue where the container's parent pointer type doesn't match the statement's container pointer type. Need to verify this type relationship is correct and consistent throughout the codebase.

expr->setName(IrContainerPasskey(), getExprName());
Complex Swap Logic

The Fusion::swap function now has complex logic to update parent pointers and ir_container_ pointers after swapping (lines 107-137). This is error-prone and could lead to dangling pointers if any step fails. The manual pointer updates for all vals and expressions (lines 121-126, 131-136) need careful review to ensure no statements are missed and all parent relationships are correctly maintained.

// We need to be careful to call IrContainer swap not unique_ptr swap, which
// will only swap the ptrs NOT the contents.
IrContainer::swap(*(a.ir_container()), *(b.ir_container()));

// Fix parent pointers after swapping containers
// After swap, each IrContainer owns a different IrContainer, so we must
// update the parent backpointers in those containers to point to their new
// owners
if (a.ir_container_) {
  // Also update all Statement ir_container_ pointers to point to new owner
  // Note: IrContainer is now in impl namespace, but Statement::ir_container_
  // is Fusion*. Since only Fusion (and its derived classes) inherit from
  // impl::IrContainer, this cast is safe.
  a.ir_container()->parent_ = &a;
  for (auto val : a.vals()) {
    val->ir_container_ = &a;
  }
  for (auto expr : a.deterministic_exprs()) {
    expr->ir_container_ = &a;
  }
}
if (b.ir_container_) {
  // Also update all Statement ir_container_ pointers to point to new owner
  b.ir_container()->parent_ = &b;
  for (auto val : b.vals()) {
    val->ir_container_ = &b;
  }
  for (auto expr : b.deterministic_exprs()) {
    expr->ir_container_ = &b;
  }
}

@mdavis36
Copy link
Collaborator Author

mdavis36 commented Jan 31, 2026

!test

@mdavis36 mdavis36 mentioned this pull request Jan 31, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mdavis36